Add IFeatureService & Related Utilities
#37
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
===========================================
+ Coverage 14.71% 37.88% +23.17%
===========================================
Files 30 38 +8
Lines 836 1127 +291
Branches 71 99 +28
===========================================
+ Hits 123 427 +304
+ Misses 695 666 -29
- Partials 18 34 +16 ☔ View full report in Codecov by Sentry. |
| /// <summary> | ||
| /// Gets a logger that is suitable for use during the bootstrapping (startup) process. | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public static ILogger GetBootstrapLogger() | ||
| { | ||
| return new LoggerConfiguration() | ||
| .MinimumLevel.Override("Microsoft", LogEventLevel.Information) | ||
| .Enrich.FromLogContext() | ||
| .WriteTo.Console() | ||
| .CreateBootstrapLogger(); | ||
| } | ||
|
|
|
No New Or Fixed Issues Found |
| { | ||
| builder.WriteTo.Console(); | ||
| } |
There was a problem hiding this comment.
The compact formatter is pretty hard to use locally, we should keep it just for production scenarios.
justindbaur
changed the title
IFeatureService & Related Utilities
There was a problem hiding this comment.
Few comments and questions but these are all great changes as I see it, and this helps clean up some of the original unknowns that we did or didn't put into practice e.g. IsOnline.
My only lingering thought is about how we establish the "known feature flags" and at the scale some projects will need. Yes, it's bad to have too many of these around but there are a lot in server today and it seems a little awkward for this to be in the service configuration / startup area of code.
| /// </summary> | ||
| /// <param name="app">The <see cref="IApplicationBuilder"/> to add the middleware to.</param> | ||
| /// <returns>A reference to <paramref name="app"/> after the operation has completed.</returns> | ||
| public static IApplicationBuilder UseFeatureChecks(this IApplicationBuilder app) |
There was a problem hiding this comment.
💭 on including "flag" in the language here.
There was a problem hiding this comment.
I like it, I changed it. Do you think we should also name the namespace/folder FeatureFlags instead of Features?
There was a problem hiding this comment.
I am flip-flopping a bit in my head over this. We have "feature checks" and we "require features", but we use feature flags to do that. This may be overcomplicating it and we just call everything one or another. Would like to hear others' opinions.
| { | ||
| if (_logger.IsEnabled(LogLevel.Debug)) | ||
| { | ||
| _logger.LogFailedFeatureCheck(failedFeature.ToString() ?? "Unknown Check"); |
There was a problem hiding this comment.
❓ How could failedFeature return a null ToString()?
There was a problem hiding this comment.
Technically ToString is marked as a nullable return: https://github.com/dotnet/runtime/blob/9ecef8c2d709b88b959d9d6a00ad62a8f72a094f/src/libraries/System.Private.CoreLib/src/System/Object.cs#L39
I kinda doubt we will run into that scenario though, I'm happy to ! away the compiler warning instead.
There was a problem hiding this comment.
Yeah, seems cleaner to me. I like having less hardcoded strings in all this.
| problemDetails.Title = "Resource not found."; | ||
| problemDetails.Status = StatusCodes.Status404NotFound; | ||
|
|
||
| // Message added for legacy reasons. We should start preferring title/detail |
There was a problem hiding this comment.
❓ Legacy? So Message isn't common anymore?
There was a problem hiding this comment.
Yeah, a lot of things in ASP.NET Core will return a ProblemDetails object unless we specifically go and and change it. So my preference would be to also make that be our return for problems. That way we can start on a unified error format.
It also sends back application/problem+json as the content type so you can introspect on that more on the client side to trust that if you got a bad status code and the content type indicates problem details, you can read the respond body like it's a problem details object and then return a more high quality error.
There was a problem hiding this comment.
Ah, I like that. Do we just get rid of the message then? I'd like to not carry in debt and just expect callers to adjust when they adopt this library.
| var httpContext = _httpContextAccessor.HttpContext; | ||
| if (httpContext == null) | ||
| { | ||
| // Likely a mistake, should we log a warning? |
There was a problem hiding this comment.
ℹ️ I would say yes -- the context should never be null, we just will have some calls that don't have a client type e.g. a user.
|
|
||
| app.MapGet("/", (IConfiguration config) => ((IConfigurationRoot)config).GetDebugView()); | ||
|
|
||
| app.MapGet("/requires-feature", (IFeatureService featureService) => |
There was a problem hiding this comment.
🌱 Should this example code be expected to declare known flags?
There was a problem hiding this comment.
It's declaring its known flags in appsettings.Development.json.
| var organizations = httpContext.User.FindAll("organization"); | ||
|
|
||
| var contextBuilder = Context.Builder(subject) | ||
| .Kind(ContextKind.Default) // TODO: This is not right |
There was a problem hiding this comment.
So this ties into your comment below about organization and service accounts missing. This is wrong because it hard codes user and I don't want it to. I totally missed putting this in my PR description because I meant to go back to this.
I want to design this in such a way that this services doesn't have to be intimately aware of all the token types and claims structures our application has or could have down the road. Instead I want all the format to have to conform to a pattern that this understands (or just be alright with it not having as much context). It feels a lot more stable in the long run but it requires changes before you could start using this. What I did for now, sub and organization (where multiple are allowed), but I would also need a claim to indicate the context kind, sub_type maybe? If we dont' think claims are the best place for this then maybe a request feature? https://learn.microsoft.com/en-us/aspnet/core/fundamentals/request-features?view=aspnetcore-8.0
There was a problem hiding this comment.
Request features feel pretty low-level and not about business logic to me, and I think this needs to be a claim. I thought we had that already but I guess not.
|
|
||
| return contextBuilder.Build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎨 Organization and service account client types are missing.
| .ApplicationId(_hostEnvironment.ApplicationName) | ||
| .ApplicationName(_hostEnvironment.ApplicationName) | ||
| .ApplicationVersion(AssemblyHelpers.GetGitHash() ?? $"v{AssemblyHelpers.GetVersion()}") | ||
| // .ApplicationVersionName(AssemblyHelpers.GetVersion()) THIS DOESN'T WORK |
There was a problem hiding this comment.
Actually looks like both version and version name don't work. I get this error:
2024-10-23 05:14:01.341 -04:00 [ApplicationInfoBuilder] WARN: Issue setting ApplicationVersion to value 'v1.0.0+8956cdbdcdef6de8baa0384fa2b6897ddf6abc5f'. Contains invalid characters.
2024-10-23 05:14:01.342 -04:00 [ApplicationInfoBuilder] WARN: Issue setting ApplicationVersionName to value '1.0.0+8956cdbdcdef6de8baa0384fa2b6897ddf6abc5f'. Contains invalid characters.
It doesn't block the use of the service though.
There was a problem hiding this comment.
Hmm, I went and looked at versions e.g. https://app.launchdarkly.com/settings/applications/Api/versions (you probably can't load this) and it seems fine:
Per https://docs.launchdarkly.com/home/observability/config-deployment#set-the-application-version I am not sure if we're actually doing something incorrectly.
There was a problem hiding this comment.
Ahh, it's because we are doing it in a different format, I was kinda confused as to how we were getting any git info at all in this project because we don't have all the stuff making it work in server here. But we have it because it was added as built-in to .NET 8 SDK.
I think we should lean on the built in format and just change the parsing of it in here.
There was a problem hiding this comment.
I agree, let's enhance our version access here in the library.
There was a problem hiding this comment.
Updated it, we do now get the full hash vs before we were trimming it to only 8 characters, would you want to trim to 8 with the new way too?
There was a problem hiding this comment.
I suppose we could offer a new property for the trimmed version too, but their docs indicate the full hash is best so I think we leave this implementation as-is.
| private TestData BuildDataSource(Dictionary<string, string> data) | ||
| { | ||
| _loggerFactory.CreateLogger("Test").LogWarning("KnownValues: {Count}", data.Count); | ||
| // TODO: We could support updating just the test data source with |
There was a problem hiding this comment.
ℹ️ I am not sure LD's SDK allows that to change once it's constructed.
There was a problem hiding this comment.
I'm going off a comment in the first example here: https://launchdarkly.github.io/dotnet-server-sdk/pkgs/sdk/server/api/LaunchDarkly.Sdk.Server.Integrations.TestData.html
But also that example wouldn't compile, so mileage may vary.
extensions/Bitwarden.Extensions.Hosting/src/Features/LoggerExtensions.cs
Outdated
Show resolved
Hide resolved
…ensions.cs Co-authored-by: Matt Bishop <mbishop@bitwarden.com>
@withinfocus I think we will need to first decide how a The thing I just definitely wanted to avoid was having this library become the repository of all feature flags. So I designed it that it could be config based but we could also just call |
|
Left a few more comments. I'm thinking that each service owns how something might ask it for flags and their values. We did this so that our clients can use our API instead of an (expensive) LD client SDK primarily. |
extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs
Outdated
Show resolved
Hide resolved
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12639
📔 Objective
Adds
IFeatureServiceand related utilities to theBitwarden.Extensions.Hostinglibrary. This is not an exact copy & paste and would be a breaking change from the current implementation.One of the biggest improvements this implementation has over the previous one is that it allows for runtime configuration changes that can be immediately consumed. It does this through using
IOptionsMonitor<>.The main consumption point of this API
IFeatureService.IsEnabled(string, bool)remains unchanged. The other consumption point[RequireFeature(string)]doesn't change from a use standpoint but behaviorally works differently behind the scenes. It uses middleware instead of an action filter to filter out endpoints that shouldn't be invoked. To use this, you have to add the new middlewareUseFeatureChecks()betweenUseRouting()andUseEndpoints(), this change was made so that we can support minimal API's which do not support action filters. This make it possible to tag and endpoint (or group) with.RequireFeature(string)to do the same thing as the attribute.The other major breaking change is how known feature flags are added. Currently they are added at
globalSettings:launchDarkly:FlagDataFilePathwhich is supposed to be a path to a file containing flags and their values or atglobalSettings:launchDarkly:FlagValues. Instead they now have to be added atFeatures:FlagValuesor throughIServiceCollection.AddFeatureFlagValues(IEnumerable<KeyValuePair<string, string>>)this method is meant to replace the this in server. Adding flag values from a file is not supported anymore, the reasoning is to limit decision fatigue. The benefit the file had was that flags could be added there and auto update the feature service, this is now done automatically, all configuration providers that support change detection can be used instead. If you want a file that ONLY contains feature flags you can add another configuration provider.IFeatureService.GetAll()gets a minor facelift as it now returns aIReadOnlyDictionary<string, JsonValue>instead ofDictionary<string, object>, this is because you should not edit the returned object andJsonValueallows you to more easily know what the type of the value is if you want to consume it.IFeatureService.IsOnline()is also removed, it's not currently consumed anywhere inserverand I don't really understand the need. TheIFeatureServicetakes care of internally making sure that it runs fine while online or offline, it's not a thing consumers should have to check.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes